Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Track HTLCs in rfq policies #1186

Merged
merged 2 commits into from
Dec 6, 2024
Merged

Track HTLCs in rfq policies #1186

merged 2 commits into from
Dec 6, 2024

Conversation

GeorgeTsagk
Copy link
Member

Description

This PR adds a simple in-memory tracking of accepted htlcs in the RFQ policy level. HTLCs that passed the policy check are now tracked by each policy, possibly affecting future policy check outcomes (depends on individual policy's implementation)

@GeorgeTsagk GeorgeTsagk self-assigned this Nov 11, 2024
@coveralls
Copy link

coveralls commented Nov 11, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 12185650868

Details

  • 0 of 141 (0.0%) changed or added relevant lines in 4 files are covered.
  • 28 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.1%) to 40.737%
Changes Missing Coverage Covered Lines Changed/Added Lines %
rfq/manager.go 0 1 0.0%
tapcfg/server.go 0 1 0.0%
chain_bridge.go 0 4 0.0%
rfq/order.go 0 135 0.0%
Files with Coverage Reduction New Missed Lines %
internal/test/helpers.go 2 86.95%
asset/mock.go 3 92.18%
commitment/tap.go 4 83.91%
asset/asset.go 4 81.26%
tappsbt/mock.go 15 85.2%
Totals Coverage Status
Change from base Build 12180566227: -0.1%
Covered Lines: 25788
Relevant Lines: 63304
💛 - Coveralls

guggero
guggero requested changes Nov 11, 2024
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned offline, we also need to track and take into account canceled HTLCs from previous attempts.
These can be subscribed to with https://lightning.engineering/api-docs/api/lnd/router/subscribe-htlc-events.

rfq/order.go Outdated Show resolved Hide resolved
rfq/order.go Show resolved Hide resolved
@dstadulis dstadulis added the rfq label Nov 11, 2024
@dstadulis dstadulis added this to the v0.5 (v0.4.2 rename) milestone Nov 11, 2024
@GeorgeTsagk GeorgeTsagk force-pushed the rfq-track-accepted-amount branch from 781102e to e559106 Compare November 13, 2024 15:32
ffranr
ffranr reviewed Nov 18, 2024
View reviewed changes
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm up-to-date with this PR. I agree with Oli's comments.

@GeorgeTsagk GeorgeTsagk force-pushed the rfq-track-accepted-amount branch from e559106 to e5edcfd Compare November 18, 2024 18:08
@GeorgeTsagk
Copy link
Member Author

@ffranr @guggero
I updated the PR to now include an htlc event subscription.
We now maintain a map of htlc circuit keys to rfq policy
When an htlc forward is failed we retrieve the htlc circuit key, fetch the corresponding policy and have it to stop tracking the htlc.

@GeorgeTsagk
Copy link
Member Author

Will also see how to add some coverage here

@ffranr
Copy link
Contributor

ffranr commented Nov 19, 2024

Will also see how to add some coverage here

@GeorgeTsagk should i review this before you add coverage or wait for that extra changes?

@GeorgeTsagk
Copy link
Member Author

Will also see how to add some coverage here

@GeorgeTsagk should i review this before you add coverage or wait for that extra changes?

You can review
Coverage will soon be added in the LitD itests

@GeorgeTsagk GeorgeTsagk force-pushed the rfq-track-accepted-amount branch from e5edcfd to 1fd418e Compare November 22, 2024 17:42
@GeorgeTsagk GeorgeTsagk mentioned this pull request Nov 22, 2024
Merged
ffranr
ffranr reviewed Nov 27, 2024
View reviewed changes
tapchannel/aux_leaf_signer.go Outdated Show resolved Hide resolved
tapchannel/aux_leaf_signer.go Outdated Show resolved Hide resolved
tapchannel/aux_leaf_signer.go Outdated Show resolved Hide resolved
tapchannel/aux_leaf_signer_test.go Outdated Show resolved Hide resolved
rfq/order.go Outdated Show resolved Hide resolved
rfq/order.go Outdated Show resolved Hide resolved
@GeorgeTsagk GeorgeTsagk force-pushed the rfq-track-accepted-amount branch 3 times, most recently from 0dd58d6 to 5c62e34 Compare November 28, 2024 18:43
@GeorgeTsagk
Copy link
Member Author

Rebased on #1224 to include manual rfq scid functionality in SendPayment

@GeorgeTsagk GeorgeTsagk requested a review from ffranr November 28, 2024 18:51
guggero
guggero reviewed Nov 29, 2024
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good. Left a couple of comments to simplify the code and make it more concurrency safe.

chain_bridge.go Show resolved Hide resolved
rfq/order.go Outdated Show resolved Hide resolved
rfq/order.go Outdated Show resolved Hide resolved
rfq/order.go Outdated Show resolved Hide resolved
rfq/order.go Outdated Show resolved Hide resolved
rfq/order.go Show resolved Hide resolved
rfq/order.go Outdated Show resolved Hide resolved
rfq/order.go Outdated Show resolved Hide resolved
rfq/order.go Outdated Show resolved Hide resolved
ffranr
ffranr reviewed Dec 2, 2024
View reviewed changes
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm up to date on this. Will wait for the revised commits based on Oli's review.

@GeorgeTsagk GeorgeTsagk force-pushed the rfq-track-accepted-amount branch from 5c62e34 to b5dfb89 Compare December 2, 2024 11:49
@GeorgeTsagk GeorgeTsagk requested review from guggero and ffranr December 2, 2024 11:50
@GeorgeTsagk
Copy link
Member Author

Thank you @ffranr @guggero

I just pushed the commits that address your feedback, also rebased on latest #1224

guggero
guggero reviewed Dec 2, 2024
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there! Concurrency is very tricky, so a couple of more comments on the mutexes.

rfq/order.go Outdated Show resolved Hide resolved
rfq/order.go Show resolved Hide resolved
rfq/order.go Show resolved Hide resolved
rfq/order.go Outdated Show resolved Hide resolved
rfq/order.go Outdated Show resolved Hide resolved
rfq/order.go Show resolved Hide resolved
@GeorgeTsagk GeorgeTsagk force-pushed the rfq-track-accepted-amount branch 2 times, most recently from a85350f to 5f3c446 Compare December 2, 2024 14:56
@GeorgeTsagk GeorgeTsagk requested a review from guggero December 2, 2024 14:58
guggero
guggero reviewed Dec 3, 2024
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending the dependent PR.

rfq/order.go Show resolved Hide resolved
@GeorgeTsagk GeorgeTsagk force-pushed the rfq-track-accepted-amount branch from 5f3c446 to 1923ca2 Compare December 4, 2024 11:32
@GeorgeTsagk
Copy link
Member Author

GeorgeTsagk commented Dec 4, 2024 •
edited
Loading

Rebased on main after merging dependent PR, PTAL

Updated LitD itests to point to 1923ca2

@GeorgeTsagk GeorgeTsagk requested a review from guggero December 4, 2024 11:52
guggero
guggero approved these changes Dec 4, 2024
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

ffranr
ffranr reviewed Dec 5, 2024
View reviewed changes
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we need

htlcToPolicy lnutils.SyncMap[models.CircuitKey, Policy]

Please see review comments.

rfq/order.go Outdated Show resolved Hide resolved
rfq/order.go Show resolved Hide resolved
rfq/order.go Outdated Show resolved Hide resolved
ffranr
ffranr approved these changes Dec 5, 2024
View reviewed changes
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will leave merging down to @GeorgeTsagk .

@GeorgeTsagk GeorgeTsagk force-pushed the rfq-track-accepted-amount branch from 1923ca2 to 8d9bb75 Compare December 5, 2024 18:16
@GeorgeTsagk GeorgeTsagk enabled auto-merge December 5, 2024 19:33
@guggero guggero disabled auto-merge December 6, 2024 08:52
@guggero guggero merged commit 8113fa8 into lightninglabs:main Dec 6, 2024
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ffranr ffranr ffranr approved these changes

@guggero guggero guggero approved these changes

Assignees

@GeorgeTsagk GeorgeTsagk

Labels
rfq
Projects
Taproot-Assets Project Board
Status: ✅ Done
Milestone
v0.5
Development

Successfully merging this pull request may close these issues.
5 participants
@GeorgeTsagk @coveralls @ffranr @guggero @dstadulis